Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable24] invalidate existing tokens when deleting an oauth client #35094

Merged
merged 6 commits into from
Jan 6, 2023
Merged

[stable24] invalidate existing tokens when deleting an oauth client #35094

merged 6 commits into from
Jan 6, 2023

Conversation

individual-it
Copy link
Contributor

@individual-it individual-it commented Nov 11, 2022 •
edited
Loading

When an oauth client is deleted all the existing tokens should be invalidated

fixes #35068

I've created a PR to stable24 and not master because of #35045 that makes it harder to test the fix

@szaimen szaimen added the 3. to review Waiting for reviews label Nov 12, 2022
@szaimen szaimen changed the title invalidate existing tokens when deleting an oauth client [stable24] invalidate existing tokens when deleting an oauth client Nov 12, 2022
@szaimen szaimen added this to the Nextcloud 24.0.8 milestone Nov 12, 2022
@szaimen
Copy link
Contributor

szaimen commented Nov 12, 2022

/backport to master

@szaimen
Copy link
Contributor

szaimen commented Nov 12, 2022

/backport to stable25

@szaimen szaimen requested review from a team, ArtificialOwl, blizzz and come-nc and removed request for a team November 12, 2022 01:09
@individual-it individual-it force-pushed the invalidateTokensWhenDeletingOAuthClient branch 2 times, most recently from 9db7278 to 6e6978b Compare November 15, 2022 07:35
@come-nc
Copy link
Contributor

come-nc commented Nov 15, 2022

Things in OC namespace should not be used in applications.
But I do not have enough knowledge about this part of the code to understand why these interfaces are in OC and not OCP and if there is a better way of doing that…

@individual-it individual-it marked this pull request as ready for review November 15, 2022 08:50
@individual-it
Copy link
Contributor Author

@come-nc I could not find anything in OCP that would do the job.
When deleting the oauth client the tokens get deleted from the oc_oauth2_access_tokens table but there are still present in the oc_authtoken table. I don't quite understand how the complete workflow works, but it looks to me that the the real authentication happens with data from oc_authtoken.

@individual-it
Copy link
Contributor Author

I believe the last test that is failing is not relevant to the code changes

@individual-it individual-it mentioned this pull request Nov 15, 2022
Merged
@julien-nc
Copy link
Member

julien-nc commented Nov 17, 2022 •
edited by nickvergessen
Loading

OC vs OCP

@nickvergessen @juliushaertl @blizzz As OC\Authentication\Token\IProvider is used by quite some apps

Can/should we consider providing an interface in the OCP namespace?

Token management

In the oauth2 app, it seems tokens are duplicated between an oauth2 table and oc_authtoken.
Same in the notifications app with oc_notifications_pushhash.
So when we delete a token in the app-specific tables, it stays in oc_authtoken if we don't take care of it (with OC\Authentication\Token\IProvider).
For example in https://github.com/nextcloud/notifications/blob/c61830aa40ae5430cc0e143cffbffbdba653384a/lib/Controller/PushController.php#L264-L271
@nickvergessen Is this intentional or does it leave a token alive while we expect it to be gone?

@nickvergessen
Copy link
Member

Same in the notifications app with oc_notifications_pushhash.
So when we delete a token in the app-specific tables, it stays in oc_authtoken if we don't take care of it (with OC\Authentication\Token\IProvider).

Notification is only additional. Not every authtoken has a pushhash entry (only the ones from devices that register for push afterwards). It is also self healing, stray entries from oc_notifications_pushhash are deleted on first push after oc_authtoken got deleted. So ignore that part. It's never used for authentication and basically only in another table to not bloat the oc_authtoken table with columns that are notification app specific.

Can/should we consider providing an interface in the OCP namespace?

I would welcome that. Also the current IToken interface does not have all columns, so apps need to even type hint to an actual implementation.

@nickvergessen nickvergessen mentioned this pull request Nov 17, 2022
Merged
@come-nc
Copy link
Contributor

come-nc commented Nov 21, 2022 

There were 3 errors:

1) OCA\OAuth2\Tests\Controller\SettingsControllerTest::testAddClient
TypeError: Argument 7 passed to OCA\OAuth2\Controller\SettingsController::__construct() must implement interface OCP\Authentication\Token\IProvider, instance of Mock_IProvider_ce21455d given, called in /home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php on line 79

/home/runner/work/server/server/apps/oauth2/lib/Controller/SettingsController.php:63
/home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php:79

2) OCA\OAuth2\Tests\Controller\SettingsControllerTest::testDeleteClient
TypeError: Argument 7 passed to OCA\OAuth2\Controller\SettingsController::__construct() must implement interface OCP\Authentication\Token\IProvider, instance of Mock_IProvider_ce21455d given, called in /home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php on line 79

/home/runner/work/server/server/apps/oauth2/lib/Controller/SettingsController.php:63
/home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php:79

3) OCA\OAuth2\Tests\Controller\SettingsControllerTest::testInvalidRedirectUri
TypeError: Argument 7 passed to OCA\OAuth2\Controller\SettingsController::__construct() must implement interface OCP\Authentication\Token\IProvider, instance of Mock_IProvider_ce21455d given, called in /home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php on line 79

/home/runner/work/server/server/apps/oauth2/lib/Controller/SettingsController.php:63
/home/runner/work/server/server/apps/oauth2/tests/Controller/SettingsControllerTest.php:79

@blizzz blizzz mentioned this pull request Nov 21, 2022
Merged
9 tasks
@individual-it
Copy link
Contributor Author

@come-nc sorry I pushed that commit yesterday just before going home from work, I know the unit tests need to be fixed / adjusted. I will try to do that today

@individual-it individual-it force-pushed the invalidateTokensWhenDeletingOAuthClient branch from 3ea6e54 to c834874 Compare November 22, 2022 08:09
@come-nc
Copy link
Contributor

come-nc commented Nov 22, 2022 •
edited
Loading 

The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh

(from drone CI, all the other tests passed without error)

@individual-it
Copy link
Contributor Author

@come-nc a lot of files got changed, see 63fedca I hope that is correct

@come-nc
Copy link
Contributor

come-nc commented Nov 22, 2022

@come-nc a lot of files got changed, see 63fedca I hope that is correct

I think it is correct, the CI is happy. It seems you ran into a composer new version, which often moves a few thing around.

@individual-it
Copy link
Contributor Author

yes I believe the last failing test is not related to my changes

@szaimen
Copy link
Contributor

szaimen commented Nov 24, 2022

This is missing one approval...

@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 25, 2022
@blizzz blizzz mentioned this pull request Nov 29, 2022
Merged
@blizzz
Copy link
Member

blizzz commented Nov 29, 2022

moving to 24.0.9

@PVince81
Copy link
Member

/rebase

@individual-it @PVince81
invalidate existing tokens when deleting an oauth client
1a33c71 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it @PVince81
public interface to invalidate tokens of user
0570327 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it @PVince81
adjust SettingsController tests
7f3badd 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it @PVince81
unit tests for Manager::invalidateTokensOfUser
bcfd571 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it @PVince81
added @SInCE tag
a0788a8 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@individual-it @PVince81
autoloaderchecker
c001c4b 
Signed-off-by: Artur Neumann <artur@jankaritech.com>
@nextcloud-command nextcloud-command force-pushed the invalidateTokensWhenDeletingOAuthClient branch from 63fedca to c001c4b Compare December 20, 2022 16:26
@skjnldsv skjnldsv mentioned this pull request Jan 6, 2023
Merged
8 tasks
@skjnldsv skjnldsv merged commit 467c213 into stable24 Jan 6, 2023
@skjnldsv skjnldsv deleted the invalidateTokensWhenDeletingOAuthClient branch January 6, 2023 07:52
@backportbot-nextcloud
Copy link

The backport to master failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

This was referenced Jan 6, 2023
Merged
Closed
@individual-it
Copy link
Contributor Author

forward port to master in #36033
forward port to stable25 in #36034

@blizzz blizzz mentioned this pull request Jan 11, 2023
Merged
5 tasks
@joshtrichards joshtrichards mentioned this pull request Jul 9, 2024
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-nc julien-nc julien-nc approved these changes

@come-nc come-nc come-nc approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@blizzz blizzz Awaiting requested review from blizzz

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst ChristophWurst is a code owner

@miaulalala miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

@nickvergessen nickvergessen Awaiting requested review from nickvergessen nickvergessen is a code owner

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Milestone
Nextcloud 24.0.9
Development

Successfully merging this pull request may close these issues.
8 participants
@individual-it @szaimen @come-nc @julien-nc @nickvergessen @blizzz @PVince81 @skjnldsv